Conversation
Signed-off-by: w01fgang <sumin@unix-center.ru>
Signed-off-by: w01fgang <sumin@unix-center.ru>
Signed-off-by: w01fgang <sumin@unix-center.ru>
Signed-off-by: w01fgang <sumin@unix-center.ru>
WalkthroughStandardizes breadcrumb typing to a new BreadcrumbData model across types, utils, and reporters; extends breadcrumb configuration (numeric keys, variadic transformers); tightens transformer validation/serializability; adds NodeReporter.createWrappedError; updates tests, docs, tooling, and exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Try as Try<T>
participant BE as BreadcrumbExtractorUtil
participant ER as ErrorReporter
participant Rep as Reporter (Node/Browser/Next.js)
participant Sentry
Caller->>Try: execute(...args).breadcrumbs(config|transformer|numeric-keyed)
Try->>BE: extract(config, args)
BE-->>Try: BreadcrumbData
Try->>ER: report(error, { breadcrumbData, message?, ignoreErrorTypes? })
alt breadcrumbData present
ER->>Rep: addBreadcrumbs(breadcrumbData, functionName)
end
alt message provided
Rep->>Rep: createWrappedError(error, message) [NodeReporter]
Rep-->>ER: wrapped Error
else
Rep-->>ER: original Error
end
ER->>Rep: report(errorToReport, tags, extras)
Rep->>Sentry: captureException(errorToReport, context)
Sentry-->>Rep: id
Rep-->>ER: id
ER-->>Try: id
Try-->>Caller: TryResult
sequenceDiagram
autonumber
actor Dev as Library User
participant Try as Try<T>
participant BE as BreadcrumbExtractorUtil
note over Dev,BE: New breadcrumb config options (numeric keys, variadic transformers)
Dev->>Try: breadcrumbs({ 0: ['id','count'], 1: transformer, 2: 'value' })
Try->>BE: extract(config, args)
BE->>BE: handle numeric keys, arrays of transformers, validate serializability
BE-->>Try: BreadcrumbData (JSON-serializable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/types.ts (1)
115-119: BreadcrumbOptions is missing variadic transformer arrays and first‑arg key arrays
BreadcrumbExtractorUtil.extractsupports both, but they’re not in the public type. Add them to avoid friction.Apply this diff:
export type BreadcrumbOptions<TArgs extends readonly unknown[]> = - | readonly BreadcrumbExtractor<TArgs>[] // Array syntax (extractors with explicit param) - | BreadcrumbConfig<TArgs> // Object syntax (map param index -> extractor) - | PositionalBreadcrumbs<TArgs>; // Positional array syntax (no string[] to force ValidateKeys) + | readonly BreadcrumbExtractor<TArgs>[] // Array syntax (extractors with explicit param) + | BreadcrumbConfig<TArgs> // Object syntax (map param index -> extractor) + | PositionalBreadcrumbs<TArgs> // Positional array syntax + // First-parameter keys with validation if the first arg is an object + | ValidateKeys<TArgs, readonly (string | number)[]> + // Variadic transformers: one transformer per arg (partial tuples allowed) + | VariadicBreadcrumbTransformers<TArgs>;
🧹 Nitpick comments (25)
.github/workflows/ci.yml (1)
17-19: Quote style only — consider using the package script for typecheck.Cosmetic change is fine. Minor consistency nit: instead of
npx tsc --noEmit(Line 25), prefernpm run typecheckto use the same command as package.json and avoid drift.tsconfig.json (1)
20-20: Including tests via tsconfig — double-check your test locations.Removing test exclusions makes sense if tests live under
src/**. If some tests are outsidesrc, they still won’t be type-checked. Adjustincludeaccordingly if needed.If tests are elsewhere, update
includeto cover them (example):{ "include": ["src/**/*", "tests/**/*", "src/__tests__/**/*"] }Optional: consider tightening types with
"noUncheckedIndexedAccess": trueand"exactOptionalPropertyTypes": trueto catch more edge cases.examples/custom-reporter.ts (1)
43-43: String literal style only — OK.Switch to double quotes avoids escapes and improves readability.
Optional: prefer the native cause API for wrapping
createWrappedError(error: Error, message: string): Error { return new Error(`${message}: ${error.message}`, { cause: error }); }Also applies to: 64-64
src/adapters/browser/reporter.ts (1)
23-26: Enrich Sentry breadcrumb metadata (category/level).Adding
categoryandlevelimproves Sentry filtering without changing behavior.Apply this diff:
Sentry.addBreadcrumb({ message: `Calling ${functionName} function`, - data, + category: 'function-call', + level: 'info', + data, });examples/comprehensive-examples.ts (3)
74-77: Prevent accidental id override in updateUser result.Spread can override earlier keys; putting
idlast ensures it’s stable.Apply this diff:
return { - id: 'updated-user', - ...userData, + ...userData, + id: 'updated-user', };
233-236: Replaceanyin breadcrumb transformers with concrete types.Keeps examples aligned with the PR’s type-safety goals.
Apply these diffs:
- 1: (opts: any) => ({ + 1: (opts: UpdateOptions) => ({ hasValidation: !!opts.validateOnly, optionCount: Object.keys(opts).length, }),- { - param: 1, - transform: (opts: any) => ({ method: opts.method || 'GET' }), - }, + { + param: 1, + transform: (opts: { method?: string }) => ({ method: opts.method ?? 'GET' }), + },- 2: (headers: any) => ({ + 2: (headers: Record<string, string> | undefined) => ({ hasHeaders: !!headers, headerCount: Object.keys(headers || {}).length, }),- transform: (updates: any) => ({ + transform: (updates: Partial<UserData>) => ({ updateFields: Object.keys(updates), fieldCount: Object.keys(updates).length, }),- transform: (h: any) => ({ + transform: (h: Record<string, string>) => ({ headerCount: Object.keys(h).length, hasAuth: !!h.authorization, }),- 3: (headers: any) => ({ headerCount: Object.keys(headers).length }), + 3: (headers: Record<string, string>) => ({ headerCount: Object.keys(headers).length }),Also applies to: 308-311, 475-478, 496-500, 632-637, 663-664
859-859: Use a stronger correlationId generator.Prefer UUID with a safe fallback.
Apply this diff:
- correlationId: Math.random().toString(36), + correlationId: globalThis.crypto?.randomUUID?.() ?? Math.random().toString(36).slice(2),CONTRIBUTING.md (2)
233-236: Fix markdownlint MD026: remove trailing colons in headings.Headings shouldn’t end with punctuation.
-#### What Happens: +#### What Happens -#### Release PR Process: +#### Release PR ProcessAlso applies to: 240-243
7-15: Sync Table of Contents with sections below.TOC is missing: Getting Help, Development Tips, Release History. Add them for parity.
src/index.ts (1)
10-12: Avoid redundant type re-exports.You both
export * from './utils/types'and explicitly re-export the same types. Prefer one to reduce churn in API docs and declaration maps.-// Export utilities -export * from './utils/types'; +// Export utilities export * from './utils/transformers'; export * from './utils/breadcrumbs'; // Explicitly export important breadcrumb types for better discoverability export type { BreadcrumbValue, BreadcrumbData, BreadcrumbTransformer, BreadcrumbOptions, BreadcrumbExtractor, PredefinedTransformerType, } from './utils/types';Also applies to: 14-22
src/utils/index.ts (1)
5-8: Deduplicate type exports for clarity.Same types are exported via
export *and explicit type block. Keep just one (suggest explicit block) to prevent redundancy.-export * from './types'; export * from './transformers'; export * from './breadcrumbs'; export * from './error-reporter'; // Explicitly export important types for easier consumption export type { BreadcrumbValue, BreadcrumbData, BreadcrumbTransformer, BreadcrumbOptions, BreadcrumbExtractor, PredefinedTransformerType, ValidateKeys, BreadcrumbConfig, PositionalBreadcrumbs, VariadicBreadcrumbTransformers, } from './types';Also applies to: 10-22
src/adapters/node/reporter.ts (2)
29-33: Don’t overwrite stack on wrapped error; rely on Error.cause.Assigning
wrapped.stack = error.stackhides the wrapper’s message/stack, reducing debuggability and Sentry grouping fidelity.causealready preserves the original stack.createWrappedError(error: Error, message: string): Error { const wrapped = new Error(message); wrapped.cause = error; - wrapped.stack = error.stack; return wrapped; }
19-26: Breadcrumb data type: consider aligning with BreadcrumbData.If
Reporter.addBreadcrumbsis typed to acceptBreadcrumbDataelsewhere, update the signature for consistency and stronger typing.src/nextjs/SentryReporter.ts (2)
48-53: Same as Node: keep wrapper stack; use cause for original.Avoid replacing the stack so the custom message appears in traces; Sentry will link the original via
cause.createWrappedError(error: Error, message: string): Error { const wrappedError = new Error(message); wrappedError.cause = error; - wrappedError.stack = error.stack; return wrappedError; }
35-38: Optional: strengthen breadcrumb data type.If
BreadcrumbDatais the canonical type, prefer it overRecord<string, unknown>for consistency.src/__tests__/Try.test.ts (2)
432-432: Redundant mock restoration inside tests.You already call
vi.restoreAllMocks()inafterEach. The per-testconsoleSpy.mockRestore()calls are redundant and can be dropped for brevity.Also applies to: 442-442, 452-452, 462-462, 482-482, 494-494, 564-564, 585-585
321-321: Duplicate test title.There are two tests named “should return the actual error”. Consider renaming one for clarity in reports.
src/__tests__/flexible-breadcrumbs.test.ts (1)
350-351: Console error spies.Looks fine; if you later add a suite-level
afterEach(vi.restoreAllMocks), you can drop manualmockRestore()calls in each test.Also applies to: 385-386, 416-416
src/core/Try.ts (4)
222-225: Remove the numeric‑key overload; it’s misleading and weakens types.This overload suggests values are
BreadcrumbTransformer<TArgs>(tuple), which isn’t what numeric‑indexed configs expect (they transform a single parameter). It’s also redundant becauseBreadcrumbOptions<TArgs>already covers the object syntax precisely.Apply this diff to drop the confusing overload:
- breadcrumbs(config: { - [key: number]: BreadcrumbTransformer<TArgs>; - }): Try<T, TArgs>;
102-114: Update docs: breadcrumbs aren’t limited to object first param anymore.Examples still claim “Only works with object first parameter,” which is no longer true given array/object/variadic transformer support across any param.
Apply this doc tweak:
- * // Chain configuration methods (breadcrumbs only available with object parameters) + * // Chain configuration methods (breadcrumbs support objects, primitives, and multiple params) * const value = await new Try(apiCall, { userId: 123, action: 'update' }) - * .report('API call failed') - * .breadcrumbs(['userId', 'action']) // Only works with object first parameter + * .report('API call failed') + * .breadcrumbs(['userId', 'action']) // extracts from first param object * .tag('component', 'user-service') * .unwrap(); - - * // Non-object parameters (breadcrumbs not available) - * const value = await new Try(processString, 'hello world') - * .report('Processing failed') - * .tag('operation', 'string-process') - * .unwrap(); + * + * // Breadcrumbs from primitives and other params via transformers + * await new Try(processOrder, 'order-123', 99.99, { tags: ['vip'] }) + * .breadcrumbs( + * (id) => ({ orderId: id }), + * (amount) => ({ amountTier: amount > 100 ? 'large' : 'small' }), + * (meta: { tags: string[] }) => ({ tagCount: meta.tags.length }), + * ) + * .unwrap();
566-567: Tiny simplification: redundant nullish coalescing.
?? undefinedis a no‑op here.Apply this diff:
- return (this.config.defaultValue as Awaited<T> | undefined) ?? undefined; + return this.config.defaultValue as Awaited<T> | undefined;
576-586: Optional: single‑flight execution to avoid duplicate work under concurrency.Two concurrent callers before
stateflips toexecutedwill executefntwice. If that’s a concern, guard with a shared in‑flight promise.I can propose a minimal single‑flight pattern if you want this property now; confirm if concurrent
.value()/.result()calls are expected in your usage.src/utils/breadcrumbs.ts (2)
199-204: Skip undefined to match key-extraction behavior
extractFromKeysdropsundefined, but the positional string mapping keeps it. Normalize to skip undefined.Apply this diff:
- if (typeof entry === 'string') { - // Map value directly under the provided key - if (this.isValidBreadcrumbValue(arg)) { + if (typeof entry === 'string') { + // Map value directly under the provided key + if (arg !== undefined && this.isValidBreadcrumbValue(arg)) { breadcrumbData = { ...breadcrumbData, [entry]: arg }; }
38-49: Avoid repeated object spreads in hot-ish loopsSpreading per entry allocates repeatedly. Prefer mutating the target with
Object.assignor direct assignment.Example:
- breadcrumbData = { ...breadcrumbData, ...transformed }; + Object.assign(breadcrumbData, transformed); - breadcrumbData = { ...breadcrumbData, ...paramData }; + Object.assign(breadcrumbData, paramData); - breadcrumbData = { ...breadcrumbData, ...data }; + Object.assign(breadcrumbData, data);Also applies to: 187-195, 207-212, 232-239
src/utils/types.ts (1)
5-6: Nit: unused imports
IsUnknownandPrimitiveare not used.Apply this diff:
-import type { Simplify, IsUnknown, Primitive } from 'type-fest'; +import type { Simplify } from 'type-fest';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.changeset/light-masks-pick.md(1 hunks).github/workflows/ci.yml(1 hunks)CONTRIBUTING.md(1 hunks)examples/comprehensive-examples.ts(27 hunks)examples/custom-reporter.ts(2 hunks)package.json(1 hunks)src/__tests__/Try.test.ts(9 hunks)src/__tests__/flexible-breadcrumbs.test.ts(11 hunks)src/adapters/browser/reporter.ts(2 hunks)src/adapters/node/reporter.ts(2 hunks)src/browser/index.ts(1 hunks)src/core/Try.ts(7 hunks)src/core/index.ts(1 hunks)src/core/reporter.ts(4 hunks)src/index.ts(1 hunks)src/nextjs/SentryReporter.ts(3 hunks)src/nextjs/index.ts(2 hunks)src/node/index.ts(1 hunks)src/utils/breadcrumbs.ts(9 hunks)src/utils/error-reporter.ts(4 hunks)src/utils/index.ts(1 hunks)src/utils/transformers.ts(5 hunks)src/utils/types.ts(3 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/__tests__/flexible-breadcrumbs.test.ts (1)
src/core/Try.ts (1)
Try(56-686)
src/core/reporter.ts (2)
src/utils/error-reporter.ts (1)
ErrorReportConfig(7-12)src/utils/types.ts (1)
BreadcrumbData(22-22)
src/utils/breadcrumbs.ts (1)
src/utils/types.ts (4)
BreadcrumbData(22-22)BreadcrumbValue(10-17)BreadcrumbExtractor(72-89)BreadcrumbTransformer(27-27)
src/utils/types.ts (3)
src/index.ts (5)
BreadcrumbValue(16-16)BreadcrumbData(17-17)BreadcrumbTransformer(18-18)PredefinedTransformerType(21-21)BreadcrumbExtractor(20-20)src/utils/index.ts (9)
BreadcrumbValue(12-12)BreadcrumbData(13-13)BreadcrumbTransformer(14-14)ValidateKeys(18-18)VariadicBreadcrumbTransformers(21-21)PredefinedTransformerType(17-17)BreadcrumbExtractor(16-16)PositionalBreadcrumbs(20-20)BreadcrumbConfig(19-19)src/core/Try.ts (1)
value(550-567)
examples/comprehensive-examples.ts (1)
src/core/Try.ts (2)
Try(56-686)result(488-490)
src/utils/error-reporter.ts (2)
src/core/reporter.ts (1)
ErrorReportConfig(6-11)src/utils/types.ts (1)
BreadcrumbData(22-22)
src/utils/transformers.ts (1)
src/utils/types.ts (5)
BreadcrumbData(22-22)BreadcrumbValue(10-17)BreadcrumbTransformer(27-27)BreadcrumbTransformationError(123-134)PredefinedTransformerType(63-67)
src/nextjs/index.ts (1)
src/core/Try.ts (1)
Try(56-686)
src/core/Try.ts (3)
src/index.ts (3)
BreadcrumbData(17-17)BreadcrumbTransformer(18-18)BreadcrumbOptions(19-19)src/utils/index.ts (4)
BreadcrumbData(13-13)BreadcrumbTransformer(14-14)VariadicBreadcrumbTransformers(21-21)BreadcrumbOptions(15-15)src/utils/types.ts (4)
BreadcrumbData(22-22)BreadcrumbTransformer(27-27)VariadicBreadcrumbTransformers(52-58)BreadcrumbOptions(115-118)
🪛 markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
233-233: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
240-240: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (19)
src/browser/index.ts (1)
14-14: Whitespace cleanup.No functional changes. Good to keep examples tidy.
Also applies to: 19-19
src/node/index.ts (1)
14-14: Whitespace cleanup.No functional changes. Looks good.
Also applies to: 19-19
package.json (1)
94-94: Approve — type-fest is correctly a devDependency; no runtime imports detected.package.json lists "type-fest": "^5.0.0" in devDependencies and a repo-wide search found no static imports, require(...) calls, or dynamic import(...) usages of "type-fest". Keep as a devDependency.
.changeset/light-masks-pick.md (1)
1-6: LGTM: clear, minimal patch changeset.Accurately describes the type-only fix.
src/core/index.ts (1)
6-6: No functional change; re-export remains correct.All good.
src/nextjs/index.ts (1)
17-21: LGTM: formatting-only generic split; static passthrough preserved.No behavior change.
src/__tests__/Try.test.ts (1)
3-4: Import path/type split LGTM.Switching
TryResultto a type-only import while keepingTryas default from../nextjsis correct and aligns with the refactor.src/__tests__/flexible-breadcrumbs.test.ts (6)
86-101: Custom transformer typing/readability LGTM.Explicit parameter typing on variadic transformers improves clarity and aligns with the new
BreadcrumbData.
117-144: Predefined extractors coverage LGTM.Good assertions across
lengthfor string/array/object; keeps behavior crisp.
182-204: toString transformer case LGTM.Covers primitive/object cases and expected string outputs.
206-228: Invalid indices handling LGTM.Nice guardrail test; confirms graceful ignore for out-of-range indices.
234-253: Object syntax with typed casts LGTM.Using
unknownat the boundary with safe casts in transformers matches intended API ergonomics.
307-343: “Skip parameter without transformer” case LGTM.Confirms omission of unconfigured params; good for privacy/signal control.
src/core/reporter.ts (1)
1-1: Type alignment with BreadcrumbData LGTM.Interfaces updated consistently;
NoopReporterremains no‑op with correct signature.Also applies to: 9-10, 30-30, 50-50
src/utils/error-reporter.ts (1)
2-2: Deprecation shim stays correct with new types.
BreadcrumbDataadoption, guarded breadcrumbs add, and wrapped error behavior look good.Also applies to: 10-10, 26-29, 47-50, 70-73
src/utils/transformers.ts (2)
39-46: value transformer may emit invalid values without the stricter checkWith the fix above, this path stays safe. If you prefer extra safety, normalize objects via shallow copy to drop non‑enumerables, but not required.
Please re-run tests that assert Date/Map/Set behavior for
as: 'value'and confirm Dates are now excluded (or update tests to reflect the stricter contract).
139-145: LGTM on index validationSimple, correct bounds check.
src/utils/types.ts (2)
34-41: ValidateKeys is defined but not wired into any optionThe previous diff wires it into
BreadcrumbOptionsso consumers get compile‑time key validation when using the “first‑arg keys” shape.
129-131: Nice error message improvementIncluding
transformerTypein the error text is helpful.
| breadcrumbs<T extends VariadicBreadcrumbTransformers<TArgs>>( | ||
| ...transformers: T | ||
| ): Try<T, TArgs>; | ||
|
|
There was a problem hiding this comment.
Fix generic shadowing in overload: breaks return type.
The method generic <T extends VariadicBreadcrumbTransformers<TArgs>> shadows the class’s T and makes the return type Try<T, TArgs> wrong (it becomes the transformer tuple type). Rename the method generic.
Apply this diff:
- breadcrumbs<T extends VariadicBreadcrumbTransformers<TArgs>>(
- ...transformers: T
- ): Try<T, TArgs>;
+ breadcrumbs<TTransformers extends VariadicBreadcrumbTransformers<TArgs>>(
+ ...transformers: TTransformers
+ ): Try<T, TArgs>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| breadcrumbs<T extends VariadicBreadcrumbTransformers<TArgs>>( | |
| ...transformers: T | |
| ): Try<T, TArgs>; | |
| breadcrumbs<TTransformers extends VariadicBreadcrumbTransformers<TArgs>>( | |
| ...transformers: TTransformers | |
| ): Try<T, TArgs>; |
🤖 Prompt for AI Agents
In src/core/Try.ts around lines 230-233, the method generic parameter `T`
shadows the class-level `T` and corrupts the return type; rename the method
generic to a non-conflicting name (e.g., `U` or `TBreadcrumbs`) and update the
parameter and constraint to `U extends VariadicBreadcrumbTransformers<TArgs>`
(or your chosen name) so the method signature reads `breadcrumbs<U extends
VariadicBreadcrumbTransformers<TArgs>>(...transformers: U): Try<T, TArgs>;`,
preserving the class `T` in the return type and updating any local references to
use the new generic name.
| if (this.isStringArray(config)) { | ||
| const firstArg = args[0]; | ||
| if (firstArg && typeof firstArg === 'object') { | ||
| breadcrumbData = this.extractFromKeys( | ||
| firstArg, | ||
| config as readonly (keyof TArgs[0])[], | ||
| firstArg as Record<string | number | symbol, unknown>, | ||
| config as readonly (string | number)[], | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Type/runtime mismatch + numeric keys unsupported in this branch
The “first‑arg keys” path only recognizes string[] via isStringArray, but you accept readonly (string | number)[] in other branches. Numeric keys here fall through to the positional branch and get ignored. Align the guard to accept numbers.
Apply this diff:
- private static isStringArray(config: unknown): boolean {
+ private static isStringArray(config: unknown): boolean {
return (
Array.isArray(config) &&
config.length > 0 &&
- config.every((el) => typeof el === 'string')
+ config.every((el) => typeof el === 'string' || typeof el === 'number')
);
}Also applies to: 129-135
| /** | ||
| * Validates if a value can be safely used as breadcrumb data | ||
| */ | ||
| private static isValidBreadcrumbValue( | ||
| value: unknown, | ||
| ): value is BreadcrumbValue { | ||
| if (value === null || value === undefined) return true; | ||
| if ( | ||
| typeof value === 'string' || | ||
| typeof value === 'number' || | ||
| typeof value === 'boolean' | ||
| ) | ||
| return true; | ||
| if (Array.isArray(value)) | ||
| return value.every((item) => this.isValidBreadcrumbValue(item)); | ||
| if (typeof value === 'object') { | ||
| return Object.values(value as Record<string, unknown>).every((v) => | ||
| this.isValidBreadcrumbValue(v), | ||
| ); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
isValidBreadcrumbValue accepts non‑plain objects (e.g., Date, RegExp)
Objects with custom prototypes pass because Object.values(new Date()) is empty, so Date gets accepted and escapes the BreadcrumbValue contract. Restrict to plain objects.
Apply this diff:
private static isValidBreadcrumbValue(
value: unknown,
): value is BreadcrumbValue {
if (value === null || value === undefined) return true;
if (
typeof value === 'string' ||
typeof value === 'number' ||
typeof value === 'boolean'
)
return true;
if (Array.isArray(value))
return value.every((item) => this.isValidBreadcrumbValue(item));
- if (typeof value === 'object') {
- return Object.values(value as Record<string, unknown>).every((v) =>
- this.isValidBreadcrumbValue(v),
- );
- }
+ if (typeof value === 'object') {
+ // Only accept plain objects
+ if (Object.getPrototypeOf(value) !== Object.prototype) return false;
+ return Object.values(value as Record<string, unknown>).every((v) =>
+ this.isValidBreadcrumbValue(v),
+ );
+ }
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Validates if a value can be safely used as breadcrumb data | |
| */ | |
| private static isValidBreadcrumbValue( | |
| value: unknown, | |
| ): value is BreadcrumbValue { | |
| if (value === null || value === undefined) return true; | |
| if ( | |
| typeof value === 'string' || | |
| typeof value === 'number' || | |
| typeof value === 'boolean' | |
| ) | |
| return true; | |
| if (Array.isArray(value)) | |
| return value.every((item) => this.isValidBreadcrumbValue(item)); | |
| if (typeof value === 'object') { | |
| return Object.values(value as Record<string, unknown>).every((v) => | |
| this.isValidBreadcrumbValue(v), | |
| ); | |
| } | |
| return false; | |
| } | |
| /** | |
| * Validates if a value can be safely used as breadcrumb data | |
| */ | |
| private static isValidBreadcrumbValue( | |
| value: unknown, | |
| ): value is BreadcrumbValue { | |
| if (value === null || value === undefined) return true; | |
| if ( | |
| typeof value === 'string' || | |
| typeof value === 'number' || | |
| typeof value === 'boolean' | |
| ) | |
| return true; | |
| if (Array.isArray(value)) | |
| return value.every((item) => this.isValidBreadcrumbValue(item)); | |
| if (typeof value === 'object') { | |
| // Only accept plain objects | |
| if (Object.getPrototypeOf(value) !== Object.prototype) return false; | |
| return Object.values(value as Record<string, unknown>).every((v) => | |
| this.isValidBreadcrumbValue(v), | |
| ); | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/utils/breadcrumbs.ts around lines 148 to 169, the current
isValidBreadcrumbValue considers non‑plain objects (e.g., Date, RegExp) valid
because Object.values on them is empty; change the object branch to only accept
plain POJOs: first verify the value is a plain object (for example via
Object.prototype.toString.call(value) === '[object Object]' or
value?.constructor === Object), and only then recurse over Object.values to
validate entries; keep existing checks for null/undefined, primitives, and
arrays unchanged.
| private static extractFromArray<TArgs extends readonly unknown[]>( | ||
| config: readonly ( | ||
| | string | ||
| | readonly string[] | ||
| | readonly (string | number)[] | ||
| | BreadcrumbExtractor<TArgs> | ||
| )[], | ||
| args: TArgs, | ||
| debug: boolean, | ||
| ): Record<string, unknown> { | ||
| let breadcrumbData: Record<string, unknown> = {}; | ||
| ): BreadcrumbData { | ||
| let breadcrumbData: BreadcrumbData = {}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Positional config supports nested key arrays at runtime, but types forbid it
extractFromArray accepts entries like readonly (string | number)[], yet PositionalBreadcrumbs<TArgs> in types excludes that shape. Either drop this runtime branch or add the type support (recommended). I’ve proposed type fixes in src/utils/types.ts.
If you keep this behavior, ensure docs mention positional “key arrays” per-arg.
Also applies to: 197-214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- Searching for relevant symbols ----"
rg -n --hidden --no-ignore -S "extractFromArray" || true
rg -n --hidden --no-ignore -S "PositionalBreadcrumbs" || true
rg -n --hidden --no-ignore -S "BreadcrumbExtractor" || true
rg -n --hidden --no-ignore -S "breadcrumbs.ts" || true
echo "---- Showing src/utils/breadcrumbs.ts (if exists) ----"
if [ -f src/utils/breadcrumbs.ts ]; then
nl -ba src/utils/breadcrumbs.ts | sed -n '1,320p'
else
echo "src/utils/breadcrumbs.ts not found"
fi
echo "---- Showing src/utils/types.ts (if exists) ----"
if [ -f src/utils/types.ts ]; then
nl -ba src/utils/types.ts | sed -n '1,400p'
else
echo "src/utils/types.ts not found"
fi
echo "---- Searching for 'Positional' occurrences ----"
rg -n --hidden --no-ignore -S "Positional" || trueLength of output: 1532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- rg matches (context) for extractFromArray & PositionalBreadcrumbs ----"
rg -n -C3 "extractFromArray|PositionalBreadcrumbs|BreadcrumbExtractor" src || true
echo "---- src/utils/breadcrumbs.ts (lines 1-260) ----"
if [ -f src/utils/breadcrumbs.ts ]; then sed -n '1,260p' src/utils/breadcrumbs.ts; else echo "src/utils/breadcrumbs.ts not found"; fi
echo "---- src/utils/types.ts (lines 1-240) ----"
if [ -f src/utils/types.ts ]; then sed -n '1,240p' src/utils/types.ts; else echo "src/utils/types.ts not found"; fiLength of output: 17928
Positional config accepts key-array entries at runtime but PositionalBreadcrumbs type forbids them
Runtime: src/utils/breadcrumbs.ts extractFromArray accepts and handles readonly (string|number)[] entries (lines ~174–214). Types: src/utils/types.ts defines PositionalBreadcrumbs as readonly (string | BreadcrumbExtractor)[] (lines ~97–100), excluding key-array entries. Either remove the Array.isArray(entry) key-array branch in extractFromArray or (recommended) update PositionalBreadcrumbs to include readonly (string|number)[] and add per-parameter key validation.
🤖 Prompt for AI Agents
In src/utils/breadcrumbs.ts around lines 174–183 and the surrounding
extractFromArray implementation (roughly lines 174–214), the function accepts
readonly (string|number)[] entries at runtime but the
PositionalBreadcrumbs<TArgs> type in src/utils/types.ts (around lines ~97–100)
forbids them; update the PositionalBreadcrumbs type to include readonly (string
| number)[] (i.e., readonly (string | number | BreadcrumbExtractor<TArgs>)[])
and then add per-parameter key validation inside extractFromArray: when
encountering an Array.isArray(entry) ensure its elements are valid keys/indices
for the current arg (check types and existence), coerce numeric strings to
numbers as needed, and only use keys that pass validation (skip or log invalid
keys). Ensure the type change is exported and adjust any call sites or tests
that assume the old type.
| static isSerializable(value: unknown): boolean { | ||
| if (value === null || value === undefined) return true; | ||
| if ( | ||
| typeof value === 'string' || | ||
| typeof value === 'number' || | ||
| typeof value === 'boolean' | ||
| ) | ||
| return true; | ||
| if (Array.isArray(value)) | ||
| return value.every((item) => this.isSerializable(item)); | ||
| if (typeof value === 'object') { | ||
| try { | ||
| JSON.stringify(value); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
isSerializable allows values outside BreadcrumbValue (e.g., Date)
JSON.stringify-based gating returns true for Dates (and other custom objects), which violates the BreadcrumbValue contract when you later cast. Replace with a structural check and restrict to plain objects.
Apply this diff:
- static isSerializable(value: unknown): boolean {
- if (value === null || value === undefined) return true;
- if (
- typeof value === 'string' ||
- typeof value === 'number' ||
- typeof value === 'boolean'
- )
- return true;
- if (Array.isArray(value))
- return value.every((item) => this.isSerializable(item));
- if (typeof value === 'object') {
- try {
- JSON.stringify(value);
- return true;
- } catch {
- return false;
- }
- }
- return false;
- }
+ static isSerializable(value: unknown): boolean {
+ if (value === null || value === undefined) return true;
+ if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean') return true;
+ if (Array.isArray(value)) return value.every((item) => this.isSerializable(item));
+ if (typeof value === 'object') {
+ // Only accept plain objects whose values are serializable
+ if (Object.getPrototypeOf(value) !== Object.prototype) return false;
+ return Object.values(value as Record<string, unknown>).every((v) =>
+ this.isSerializable(v),
+ );
+ }
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static isSerializable(value: unknown): boolean { | |
| if (value === null || value === undefined) return true; | |
| if ( | |
| typeof value === 'string' || | |
| typeof value === 'number' || | |
| typeof value === 'boolean' | |
| ) | |
| return true; | |
| if (Array.isArray(value)) | |
| return value.every((item) => this.isSerializable(item)); | |
| if (typeof value === 'object') { | |
| try { | |
| JSON.stringify(value); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| return false; | |
| } | |
| static isSerializable(value: unknown): boolean { | |
| if (value === null || value === undefined) return true; | |
| if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean') return true; | |
| if (Array.isArray(value)) return value.every((item) => this.isSerializable(item)); | |
| if (typeof value === 'object') { | |
| // Only accept plain objects whose values are serializable | |
| if (Object.getPrototypeOf(value) !== Object.prototype) return false; | |
| return Object.values(value as Record<string, unknown>).every((v) => | |
| this.isSerializable(v), | |
| ); | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/utils/transformers.ts around lines 64-83, isSerializable currently uses
JSON.stringify which returns true for Dates and other non-plain objects; change
it to only allow primitive types, arrays (recursively), and plain objects.
Implement a structural plain-object check (e.g.
Object.prototype.toString.call(value) === '[object Object]' or
Object.getPrototypeOf(value) === Object.prototype) instead of JSON.stringify,
recurse into plain-object properties to ensure all values are serializable, and
return false for Date, RegExp, class instances, and other non-plain objects.
Signed-off-by: w01fgang <sumin@unix-center.ru>
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/__tests__/Try.test.ts (7)
442-442: Same nit as above for console spy.
452-452: Same nit as above for console spy.
462-462: Same nit as above for console spy.
482-482: Same nit as above for console spy.
494-494: Same nit as above for console spy.
563-563: Same nit as above for console spy.
584-584: Same nit as above for console spy.
🧹 Nitpick comments (2)
.husky/pre-commit (1)
4-4: Prefer local binary; avoid network fetch.Use npx with --no-install (or your PM’s exec) to prevent unexpected registry hits.
Apply this diff:
-npx lint-staged +set -e +npx --no-install lint-stagedsrc/__tests__/Try.test.ts (1)
432-432: Minor: prefer explicit void return in mock to reduce lint noise.Use
() => undefinedinstead of an empty block for clarity; also you can rely onafterEach(vi.restoreAllMocks)and drop per-testmockRestore().-const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); +const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => undefined);Optional helper to dedupe across tests (place near the top of the file):
async function withSilencedConsoleError<T>(fn: () => Promise<T> | T): Promise<T> { const spy = vi.spyOn(console, 'error').mockImplementation(() => undefined); try { return await fn(); } finally { spy.mockRestore(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.husky/pre-commit(1 hunks)package.json(2 hunks)src/__tests__/Try.test.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (4)
.husky/pre-commit (2)
1-4: LGTM: Standard Husky pre-commit hook.Sourcing Husky runtime and running lint-staged is correct.
1-4: Ensure the hook is executable and husky/lint-staged are wired — verified..husky/pre-commit is executable (100755); package.json scripts.prepare = "husky install"; lint-staged present at ^16.1.6.
src/__tests__/Try.test.ts (2)
39-39: Readonly class field is a nice safety improvement.Immutability on
nameavoids accidental mutation in tests. LGTM.
3-4: Approve: Type-only import split is correct; no stale imports found. Verified src/core/Try.ts exports type TryResult and src/nextjs/index.ts still exports default Try; no imports of TryResult from ../nextjs detected.



Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores